fix: serve the pdf.js data files from a controller instead of as plain files - #1547
Open
ByteSide wants to merge 1 commit into
Open
fix: serve the pdf.js data files from a controller instead of as plain files#1547ByteSide wants to merge 1 commit into
ByteSide wants to merge 1 commit into
Conversation
…n files pdf.js loads its localization (locale.json and <lang>/viewer.ftl), the CMaps for CJK documents (*.bcmap) and the standard fonts (*.pfb, *.ttf) at runtime, relative to a base URL. They were linked as plain files below apps/files_pdfviewer/js/pdfjs/web/, which only works when the web server serves those file extensions directly. With pretty URLs enabled on Apache, everything not on the static extension list of the front controller rule is handed to index.php, which answers 404 for all four: the viewer then shows the alt text dialog instead of the document, and CJK documents render blank. The official Docker image enables pretty URLs, so this affects every Apache install based on it. The files are now served by AssetController through a route below /apps/files_pdfviewer/assets/, restricted to the three directories and the file extensions pdf.js needs from them. The version hash the viewer already appends to its other assets is part of the URL, so the responses can be cached as immutable for the same lifetime the web server gives the other assets. The template points cMapUrl, the l10n link and the newly set standardFontDataUrl at that route. Resolves: nextcloud/server#63512 Resolves: nextcloud#873 Signed-off-by: Stefan - ByteSide.io <sr@byteside.io>
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pdf.js loads its localization (
locale.jsonand<lang>/viewer.ftl), the CMaps for CJK documents (*.bcmap) and the standard fonts (*.pfb,*.ttf) at runtime, relative to a base URL. They were linked as plain files belowapps/files_pdfviewer/js/pdfjs/web/, which only works when the web server serves those file extensions directly.With pretty URLs enabled on Apache, everything not on the static extension list of the generated front controller rule (
css|js|mjs|svg|…) is handed toindex.php, which answers404for all four. The viewer then shows the alt text dialog instead of the document (nextcloud/server#63512, reported on 34.0.3 and 33.0.8), and CJK documents render as blank pages (#873, open since 2023). The official Docker image enables pretty URLs, so this affects every Apache install based on it.This PR serves the files from an
AssetControllerthrough/apps/files_pdfviewer/assets/{version}/{path}instead:locale/,cmaps/andstandard_fonts/belowjs/pdfjs/web/are reachable, and inside them only the extensions pdf.js needs (json,ftl,bcmap,pfb,ttf); every segment is checked and the resolved path is verified to stay inside the directory{version}segment is the hash the viewer already appends to its other assets, so the responses are cached as immutable for the same lifetime.htaccessgives the other viewer assetstemplates/viewer.phppointscMapUrland the l10n<link>at the route, and now also setsstandardFontDataUrl, which pdf.js otherwise derives relative toviewer.mjs(the same 404 for the.pfbfonts)This is the approach suggested in nextcloud/server#63947: keep the web server configuration as it is and let the app serve what it needs within the framework, rather than growing the static extension allow list with every pdf.js release (
.properties→.json/.ftl→.bcmap/.pfb).Testing
Against
nextcloud/servermaster in thenextcloud:34-fpmimage (PHP 8.5), with the app mounted intoapps/:composer run test:unit: 20 tests, 83 assertions, green (17 of them new)psalm: no errors;php-cs-fixer: nothing to fixphp -Sagainstindex.php, i.e. the worst case where nothing is served statically:locale/locale.json,locale/pt-BR/viewer.ftl,cmaps/Adobe-Japan1-UCS2.bcmap,standard_fonts/FoxitSerif.pfbandstandard_fonts/LiberationSans-Regular.ttfall return200with the expected content type,Content-LengthandCache-Control: private, max-age=15778463, immutable; the CMap is byte-identical to the file on disk.locale/../../build/pdf.mjs,../viewer.mjs,locale/README.mdand a missing file return404.data-cmapurl,data-standardfontdataurland the l10n link on the new route, with the version hash in the pathThe compiled bundles are included (node 24 / npm 11 as in
engines); the pdf.js dist re-fetched bynpm run buildcame back identical, so only the two app chunks changed.Checklist
stable34andstable33carry the same template and pdf.js 4.10.38, please backport if you agree